Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: drop MapLibreGL naming and deprecate export default #567

Merged
merged 24 commits into from
Dec 23, 2024

Conversation

KiwiKilian
Copy link
Collaborator

@KiwiKilian KiwiKilian commented Dec 20, 2024

Merge #563 first.

As recommended by React Native Builder Bob I've removed default exports and generally removed wherever possible from the library. The resulting breaking changes are documented in the migration guide.

BREAKING CHANGE: Remove MapLibreGL default export, use `import * as MapLibreRN` instead
# Conflicts:
#	docs/components/BackgroundLayer.md
#	docs/components/CircleLayer.md
#	docs/components/FillExtrusionLayer.md
#	docs/components/FillLayer.md
#	docs/components/HeatmapLayer.md
#	docs/components/LineLayer.md
#	docs/components/MapView.md
#	docs/components/RasterLayer.md
#	docs/components/SymbolLayer.md
#	docs/docs.json
#	packages/examples/src/examples/UserLocation/FollowUserLocationRenderMode.tsx
#	src/MLRNModule.ts
#	src/components/MapView.tsx
@KiwiKilian KiwiKilian changed the title feat: remove MapLibreGL and default export feat: drop MapLibreGL naming and default export Dec 20, 2024
@KiwiKilian KiwiKilian changed the title feat: drop MapLibreGL naming and default export feat: drop MapLibreGL naming and export default Dec 20, 2024
@KiwiKilian KiwiKilian marked this pull request as ready for review December 22, 2024 18:14
@KiwiKilian KiwiKilian requested a review from tyrauber December 22, 2024 18:14
@tyrauber
Copy link
Collaborator

I can understand migrating to named exports and updating the docs accordingly, but I am concerned about the extent of breaking changes, which are more significant than those listed in the migration docs.

Because of the gap between 9.0 and 10.0, and the number of people using 10.0 beta in production, doing a patch bump on beta with this extent of breaking changes would break any app using beta in production.

That is unacceptable IMO. My preference would be to support both for the 10.0 release.

For example:

export const offlineManager = new OfflineManager();
export { offlineManager as OfflineManager };
export default offlineManager

Would enable the new api, but allow current functionality to work without breaking changes. We can add a deprecation notice if need be, and remove them with the next Major release, but I don't think we can break this much API with a patch.

@KiwiKilian
Copy link
Collaborator Author

KiwiKilian commented Dec 22, 2024

I see your concern. These are the points, why it would rather break stuff now:

  • The fastest migrations for anyone just wanting to upgrade is:
    • Search and replace of this:
    - import MapLibreGL from "@maplibre/maplibre-react-native";
    + import * as MapLibreGL from "@maplibre/maplibre-react-native";
    • Uppercasing the first letter of any usages of locationManager, offlineManager and snapshotModule
    • I think this is possible quite fast in any project size, what other extent of necessary changes do you see?
  • We also added breaking changes in the previous beta releases
  • Anyone using the betas in production should have checks in place, which would notify that stuff isn't working anymore – any linting/TypeScript setup should break the build after updating
  • Less churn for not having the deprecations around, less communication necessary

But I'm also fine to keep old exports as deprecated, just let me know which path you want to go.

@tyrauber
Copy link
Collaborator

What do we gain from doing this? Other than following builder-bob recommendations. Is it more performant? Is it less error-prone? Does it provide greater functionality? Does it simplify the build configuration?

While we have made minor api changes in alpha & beta, there were reasons for each. This breaks literally everything. Not just the methods you mentioned, but also AnimatedShape, Logger, LocationManager, SnapshotManager, etc. Updated exports across hundreds of files.

Someone does a patch version bump on 10.0.0.beta, and if they are lucky, CI/CD catches it. It may be an easy migration with a couple find and replaces, but it is completely unnecessary. Why cause this level of frustration?

I am sorry. I just don't get it. Why is this necessary?

@KiwiKilian
Copy link
Collaborator Author

KiwiKilian commented Dec 22, 2024

To me personally there is a handful of reasons to drop the default export. But I understand, these might be very subjective arguments. Therefore I will revert the breaking changes to deprecations. The following is the reasoning, why I want to deprecate the default exports. Please let me know if you are opposing a deprecation.

Sorry, I should have communicated the reasonings behind this much more clearly.


First of all, I think it's good to distinguish from the MapLibreGL branding, as it's dropped by MapLibre Native and can be confused with maplibre-gl-js. While we could adopt a new default export like MapLibreRN, it might be much more readable on docs and examples without a namespace prefixed everywhere.

Most React libraries which export components use named imports in their docs/examples. expo-* modules/packages are always using either named imports or * as namespace import. Basically it feels off the way we are currently presenting the usages in the docs/examples.

But of course this doesn't require removing the user facing default export. But by unifying the way this library is used, reports and discussions in this repository could be more readable and more understandable to everyone.


Taken from the Builder Bob docs:

Avoid using default exports in your library. Named exports are recommended. Default exports produce a CommonJS module with a default property, which will work differently than the ESM build and can cause issues.

Here are some clarifications of the possible issues.


This breaks literally everything. Not just the methods you mentioned, but also AnimatedShape, Logger, LocationManager, SnapshotManager, etc. Updated exports across hundreds of files.

I want to clarify: Changing the individual exports within src/components and src/modules from default to named has no effect towards users of the library. This only effects usage internally. The benefit of having named exports internally is, that changing a component name, is reflected everywhere by design.

For library users, mainly the changes in src/index.ts and src/MapLibreRN.ts (deleted in the current state) are relevant. As long as the previous name matches the actual component/module name. So I'm not sure what you mean by methods I didn't mention. For most consumers it mostly requires a change on the imports in adding * as, if they don't want to adopt named imports. Only other necessary change would be uppercasing the three singleton modules.


To conclude, changing the user facing exports in src/index.ts and src/MapLibreRN.ts might only be justified by the possible ESM issues.

@KiwiKilian KiwiKilian changed the title feat: drop MapLibreGL naming and export default feat: drop MapLibreGL naming and deprecate export default Dec 23, 2024
@KiwiKilian
Copy link
Collaborator Author

I've pushed the deprecations, now this PR only deprecates exports and introduces no new breaking changes.

@tyrauber
Copy link
Collaborator

Perfect! Thank you.

# Conflicts:
#	android/build.gradle
@KiwiKilian KiwiKilian merged commit aa0c73d into maplibre:beta Dec 23, 2024
13 checks passed
@KiwiKilian KiwiKilian deleted the feat/remove-maplibre-gl branch December 23, 2024 15:04
github-actions bot pushed a commit that referenced this pull request Dec 23, 2024
# [10.0.0-beta.13](v10.0.0-beta.12...v10.0.0-beta.13) (2024-12-23)

### Features

* drop `MapLibreGL` naming and deprecate `export default` ([#567](#567)) ([aa0c73d](aa0c73d))
Copy link

🎉 This PR is included in version 10.0.0-beta.13 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants